Pin REPACK's actual behaviour, and name it in the error (#399) - #409
Conversation
design/PG18_19_OPPORTUNITIES.md concluded that REPACK "should work" on a columnar
table, because REPACK reuses the CLUSTER machinery and pgColumnar implements
relation_copy_for_cluster.
It does not work. The callback is registered and is a stub:
pgcolumnar_relation_copy_for_cluster(...)
{
COLUMNAR_UNSUPPORTED("CLUSTER / VACUUM FULL");
}
So "we implement that callback" was true of the symbol and false of the behaviour,
which is the gap a suite closes and a grep does not.
Measured on 19beta2: REPACK, REPACK ... USING INDEX, REPACK (VERBOSE), CLUSTER and
VACUUM FULL all raise on a columnar table, while REPACK succeeds on a heap table on
the same build. REPACK CONCURRENTLY is not the syntax; it is REPACK (CONCURRENTLY),
and that form fails on heap too, so it is not a columnar behaviour and the suite
records it rather than attributing it to us.
The user-visible defect is the message. A PostgreSQL 19 user types REPACK and is
told that CLUSTER / VACUUM FULL is unsupported: two commands they did not type,
and on 19 the ones REPACK replaced. It now names REPACK on 19 and hints at
pgcolumnar.vacuum(), which does the work. This is a spelling difference rather
than a missing capability, and the error is where someone will look for that.
test/native_repack.sh, registered, gated to 19 with a VISIBLE skip below it. It
asserts the behaviour that exists rather than the behaviour that was hoped for: the
failure of every spelling, that the message names the command, that the table is
unharmed afterwards (rows, content hash, storage options, access method), and that
pgcolumnar.vacuum() succeeds and preserves content. The heap control is there so a
future "REPACK broke" can be told apart from "REPACK never worked here".
If someone later implements relation_copy_for_cluster these checks fail, and that
is the intended signal to rewrite them to assert success.
Proven by removal: on unmodified main only the "names REPACK" check fails, and
every other check passes on both builds, because they pin behaviour rather than
detect the change.
docs/limitations.md now lists REPACK beside CLUSTER and VACUUM FULL. The design
document is corrected in place rather than quietly, since it is what produced the
wrong expectation.
Gate: five-major matrix, ALL VERSIONS PASSED, native_repack PASS on all five.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
The finding is right and the correction is overdue. One change: the CONCURRENTLY case pins the wrong reason.
Verified everything else independently on the bench, both majors, before this.
What holds
The version gate works, exercised on both sides of the #if:
pg19a VACUUM FULL r ERROR: columnar: REPACK, CLUSTER and VACUUM FULL are not supported yet
pg18a VACUUM FULL r ERROR: columnar: CLUSTER and VACUUM FULL are not supported yet
The hint makes a promise, so I tested it rather than trusting it. On both majors, after
deleting half the rows, pgcolumnar.vacuum() succeeds, leaves 25,000 rows and the table
undamaged. The "spelling difference, not a missing capability" claim is accurate.
REPACK h on heap succeeds on the same build, so the control does its job.
The correction to PG18_19_OPPORTUNITIES.md is the right shape, and marking the entry
CORRECTED in place rather than deleting it is better than what I would have done.
The change I am asking for
REPACK (CONCURRENTLY) failed on both heap and columnar in your table, and you concluded
it "fails on heap too, so it is not a columnar behaviour" and asserted the error without
investigating further.
That conclusion holds only because the fixture has no primary key. The error says so:
ERROR: cannot execute REPACK (CONCURRENTLY) on relation "r"
HINT: Relation "r" has no identity index.
That is a precondition, not a verdict. With a primary key on 19beta2, wal_level=logical:
REPACK (CONCURRENTLY) hk [heap] ok, no error
REPACK (CONCURRENTLY) ck [columnar] ERROR: columnar: REPACK, CLUSTER and VACUUM FULL
are not supported yet
Heap succeeds. Columnar fails, and it fails with our error. So it is a columnar
behaviour after all, and the reason we could not see that is that the fixture never met
the precondition.
Two consequences:
- The suite currently pins "REPACK (CONCURRENTLY) errors because there is no identity
index". Add a primary key to that fixture and the error changes, the test still
passes, and it now means something different. A test whose meaning depends on an
incidental property of its fixture is the kind that survives the thing it was written
to catch. - The PR body and the changelog say this form is not ours. On a table that can actually
be repacked concurrently, it is.
Suggested: a second fixture with a primary key, asserting that heap succeeds and
columnar raises our error, with the no-PK case kept as it is and labelled as the
precondition check. That distinguishes the two failures, which is the whole point.
Good news that came out of the same run
Your fourth bullet worried about a half-finished concurrent rewrite. It does not happen:
after the failed REPACK (CONCURRENTLY), the columnar table still had all 20,000 rows
with every value intact. Worth asserting, since you were right to worry and the answer is
reassuring.
Requesting changes only for the fixture. Everything else I would merge as is.
Review catch from ChronicallyJD, and it is the good kind: my check passed for a reason that had nothing to do with what it claimed. I asserted that REPACK (CONCURRENTLY) "fails on columnar and on heap alike, so it is not ours". Both did fail, but only because the fixture had no identity index, which is a precondition PostgreSQL enforces before the access method is consulted. A test whose meaning rests on an incidental property of its fixture keeps passing after the behaviour it was written to catch has changed. With a primary key and wal_level=logical, heap succeeds and a columnar table raises OUR error. So the concurrent form is a columnar behaviour, and the earlier conclusion was wrong in the PR body, the changelog and the design note. All three are corrected. The suite now carries both fixtures and asserts the distinction rather than the outcome: without an identity index the refusal is PostgreSQL's own precondition error, and with one it is ours. Two different failures that a single "it errors" check conflated. Also asserts what the review found on the way: after the refused concurrent repack the columnar table is undamaged, rows and content hash unchanged. That was the fourth bullet's worry on #399 and the answer is reassuring, so it is pinned. The cluster needs wal_level=logical and room in the lock table for the concurrent form to be reachable at all; without them it fails for reasons unrelated to the access method, which is the same trap one level down. 19 checks, all passing on PG19, skipping visibly below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md # test/run_all_versions.sh
You were right, and the check was passing for the wrong reasonFixed and pushed. This is the good kind of catch: my assertion held, and it held I wrote that Reproduced your result in my container, with So it is a columnar behaviour. Corrected in three places, since I had it wrong in all What the suite asserts nowBoth fixtures, and the assertion is the distinction rather than the outcome:
A single "it errors" check conflated those two, which was the whole defect. And your good news is pinnedAfter the refused concurrent repack the columnar table is undamaged: rows and content Two environmental preconditions, which are the same trap one level downThe suite now sets 19 checks, all passing on PG19, visible skip below it. Also merged main in
Will post that result here when it finishes. Ready for another look. |
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approved. The new assertion is better than what I suggested.
I proposed a second fixture asserting that heap succeeds and columnar raises ours. You did
that and added the part I missed: the no-identity-index case now fails if it sees our
message there. So the suite asserts the distinction rather than the outcome, and a single
"it errors" check can no longer stand in for two different reasons.
That is the correct shape. My version would still have passed if our error had started
appearing on the precondition path.
Correcting it in all three places, including the design note, is right. A test that pins
the truth while a document beside it states the opposite is how the REPACK claim survived
in the first place.
One collision to sort out, not a review objection
We are both editing design/PG18_19_OPPORTUNITIES.md right now.
My #411 renames it to design/POSTGRESQL_VERSION_ADOPTION.md and rewrites item 5 with
your finding, because the old name pinned the file to two majors. Your #409 corrects item 5
in place under the old name.
Whichever merges second will conflict, and a rename against an edit is the kind git
resolves badly.
Suggestion: merge this one first. It is the correctness change and it should not wait
on a documentation rename. I will rebase #411 on top, take your wording for item 5 over
mine where they differ, and carry it into the renamed file. That way your text is the one
that survives and I do the reconciling.
Say if you would rather it went the other way and I will hold #411 instead.
Verified
Your fix reproduces what I measured: with an identity index, REPACK (CONCURRENTLY)
succeeds on heap and raises our error on columnar. Pinning the undamaged-table result is
worth having, since the worry that prompted it was reasonable.
|
Matrix on the merged tree (with #408 in it), five majors: Both suites green on every major, and |
…er (commandprompt#395) The roadmap's Done table said PostgreSQL integration was covered while its adoption list four screens down still said go and confirm generated columns and temporal constraints, and go and investigate REPACK. Whoever marked read stream shipped left the three below it stale. The Done table was the one telling the truth. test/generated_columns.sh and test/temporal.sh both exist and run in the matrix, and REPACK was investigated and does not work on a columnar table (commandprompt#399, commandprompt#409). The list now says so. Verified rather than assumed, because the first draft of this commit got it wrong in the same direction: it recorded the all-null chunk for a virtual generated column as an open write-path finding. columnar_write_state.c skips attgenerated == 'v' entirely and generated_columns.sh pins it, so that shipped too, and the bullet now says what the code does. Also, the reachability check owned half its property. It grepped mkdocs.yml and never asked whether docs/roadmap.md existed, so deleting the page while keeping the nav entry passed. mkdocs build --strict catches that in docs.yml, but a property asserted half here and half in a workflow is how the missing half goes unnoticed. Both halves are now stated where the property is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqprqkCXuH8SegiZejE1Tw
2 participants
Closes #399. @ChronicallyJD for review. No benchmark needed.
The finding changed the deliverable
You expected a test. What the test found is that the design document's conclusion is
wrong, so this PR is a correction plus the test.
design/PG18_19_OPPORTUNITIES.mdsaid REPACK "should work" because it dispatchesthrough
relation_copy_for_clusterand pgColumnar implements that callback. Thecallback is registered and is a stub:
"We implement that callback" was true of the symbol and false of the behaviour. That is
exactly the gap you said a suite closes and a grep does not, and it is why your instinct
to distrust reasoning-about-dispatch was right.
Measured on 19beta2
REPACK rREPACK r USING INDEXREPACK (VERBOSE) rREPACK CONCURRENTLY rREPACK (CONCURRENTLY) rVACUUM FULL/CLUSTERpgcolumnar.vacuum('r')Two things that change your third bullet:
REPACK CONCURRENTLYis not the syntax(it is
REPACK (CONCURRENTLY), an option), and that form fails on heap too, so itis not a columnar behaviour. The suite records it rather than attributing it to us.
The actual user-visible defect
A 19 user types
REPACKand is toldCLUSTER / VACUUM FULL is not supported yet: twocommands they did not type, and on 19 the ones REPACK replaced. The message now names
REPACKon 19 and hints atpgcolumnar.vacuum().This is a spelling difference, not a missing capability -- the operation is
available under another name -- so I did not implement
relation_copy_for_clusterunder this issue.
The suite
test/native_repack.sh, registered, 19-gated with a visible skip below it (verifiedon PG18:
SKIP REPACK requires PostgreSQL 19, exit 0).It asserts the behaviour that exists, not the behaviour that was hoped for: every
spelling fails, the message names the command, the table is unharmed afterwards (rows,
content hash, storage options, access method), and
pgcolumnar.vacuum()succeeds andpreserves content. If someone later implements the callback these checks fail, and
that is the intended signal to rewrite them to assert success.
Proven by removal: on unmodified main only the "names REPACK" check fails. Every
other check passes on both builds, because they pin behaviour rather than detect the
change.
Docs
docs/limitations.mdlists REPACK beside CLUSTER and VACUUM FULL.CHANGELOG.mdentry for the message.It is what produced the wrong expectation, and leaving it would produce it again.
Gate
Five-major matrix,
ALL VERSIONS PASSED,native_repack=PASSon all five.🤖 Generated with Claude Code